adding dither function #33
Conversation
There was a problem hiding this comment.
Thank you very much for your contribution.
Sorry it took so long for someone to review your PR. This is just a read-only mirror of the upstream Mercurial repository. (But we try to port PR from here to the upstream repository if possible.)
I haven't checked the new function yet. But I left a few comments about the coding style that is used in Octave.
For the time being, I only took a look at the function documentation and had a very quick look at the function bodies (mostly for style).
I didn't bother to leave a comment at each and every line where the style should be adapted. But I tried to describe the coding style at some examples.
Updated comments and formatting in the dither.m file to comply with style guidelines.
scripts/image/dither.m
Outdated
| RGB = cat (3, RGB, RGB, RGB); # Duplicate grayscale to RGB | ||
| if nargin < 2, | ||
| map = [0 0 0; 1 1 1]; % binary (black and white) colormap | ||
| map = [0 0 0; 1 1 1]; # binary (black and white) colormap |
There was a problem hiding this comment.
Use two spaces before inline comments here and anywhere else where applicable.
|
Thank you again for the modifications. Please, also add the new file to the build system. (So, it will be included when creating a source tarball). For that, add the new file to the list starting here: octave/scripts/image/module.mk Line 15 in c77d758 (That list is in lexical order afaict.) Also, please add the documentation for the new file to the manual. A good place might be around here: octave/doc/interpreter/image.txi Lines 143 to 154 in c77d758 But maybe, you'll find a better location for that. |
scripts/image/dither.m
Outdated
| ## | ||
| ## This program is distributed in the hope that it will be useful, | ||
| ## but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| ## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
There was a problem hiding this comment.
The "standard" copyright statement in other files has two spaces after the full-stop in this line.
scripts/image/dither.m
Outdated
| ## @deftypefn {Function File} {@var{X} = } dither (@var{RGB}, @var{map}) | ||
| ## @deftypefnx {Function File} {@var{X} = } dither (@var{RGB}, @var{map}, @var{Qm}, @var{Qe}) | ||
| ## @deftypefnx {Function File} {@var{BW} = } dither (@var{I}) | ||
| ## Quantize an image, using dithering to increase the apparent color resolution. |
There was a problem hiding this comment.
I'm not a native speaker. But I think, we don't need the comma in this sentence.
scripts/image/dither.m
Outdated
| ## [ x 7] | ||
| ## [3 5 1] / 16 | ||
| ## It uses a raster scan and no weight renormalization at boundaries. | ||
| ## The default values are used: @var{Qm}=5, and @var{Qe}=8. |
There was a problem hiding this comment.
No comma before "and" when listing less than three elements.
scripts/image/dither.m
Outdated
| ## It uses a raster scan and no weight renormalization at boundaries. | ||
| ## The default values are used: @var{Qm}=5, and @var{Qe}=8. | ||
| ## | ||
| ## @var{RGB} is a mxnx3 array with values in [0, 1] (double) or [0, 255] (uint8). |
There was a problem hiding this comment.
Did you check how texinfo handles this? Do we need something like @code{[0, 1]} here?
scripts/image/dither.m
Outdated
| ## color axis (R, G, B). @var{Qm} defines the precision of the color space | ||
| ## discretization used to map input RGB values to those colors available in the | ||
| ## colormap. @var{Qe} is the number of quantization bits for the color space | ||
| ## error calculations in the Floyd-Steinberg error diffusion algorithm. |
There was a problem hiding this comment.
Remove whitespace at end of line here and anywhere else where applicable.
scripts/image/dither.m
Outdated
| ## Ref [1] Floyd, R. W., and Steinberg, L., An Adaptive Algorithm for Spatial | ||
| ## Gray Scale, International Symposium Digest of Technical Papers, Society for | ||
| ## Information Displays, 1975, p. 36. | ||
| ## Ref [2] Ulichney. R., Digital Halftoning, The MIT Press, 1987. |
There was a problem hiding this comment.
Consider using the @cite texinfo macro.
Updated comments for clarity and consistency in the dither function.
scripts/image/dither.m
Outdated
| ## RGB2INDLUT Map an RGB pixel to the nearest colormap index using a lookup table. | ||
| ## id = RGB2INDLUT (pixel, map) returns the 1-based index of the closest color | ||
| ## in the colormap 'map' for the input RGB pixel (1x3 vector), using a quantized | ||
| ## inverse colormap with 2^5 bins per RGB axis. | ||
| ## id = RGB2INDLUT (pixel, map, Qm) uses Qm bits for quantization per RGB axis. | ||
| ## | ||
| ## Inputs: | ||
| ## pixel: 1x3 vector [R, G, B], values in [0, 1] (double) or [0, 255] (uint8). | ||
| ## map: c-by-3 matrix, each row an RGB triplet in [0, 1] (double). | ||
| ## Qm: Number of quantization bits per axis (default: 5). | ||
| ## | ||
| ## Output: | ||
| ## id: Index (1-based) into the colormap 'map' for the closest color. | ||
| ## | ||
| ## Notes: | ||
| ## - Uses a persistent lookup table (LUT) for speed. | ||
| ## - LUT is recomputed if map or Qm changes. | ||
| ## - Warns if Qm is too large (>8) due to memory constraints. | ||
| ## - Assumes input pixel and map are properly scaled (pixel auto-scaled if needed). |
There was a problem hiding this comment.
Even if the help text of sub-functions might not be directly accessible, it might be a good idea to use texinfo syntax here for consistency.
scripts/image/dither.m
Outdated
| ## It uses a raster scan and no weight renormalization at boundaries. | ||
| ## The default values are used: @var{Qm}=5 and @var{Qe}=8. | ||
| ## | ||
| ## @var{RGB} is a mxnx3 array with values in @code{[0, 1]} (double) or @code{[0, 255]} (uint8). |
There was a problem hiding this comment.
Break lines in documentation before 80 (or 74) characters. (Exception are lines with @deftypefn or @deftypefnx.)
scripts/image/dither.m
Outdated
| error ('rgb2indLUT: Colormap must be a c-by-3 matrix'); | ||
| endif | ||
| if (nargin < 3) | ||
| Qm = 5; % Default quantization bits |
There was a problem hiding this comment.
I thought that you fixed that at some point:
Inline comments should use #.
There should be two spaces before # for inline comments.
There are a couple more inline comments with % a few lines down.
2 participants
I've made an implementation of the dither function, compatible with Matlab implementation (https://www.mathworks.com/help/matlab/ref/dither.html). In Matlab it seems not to be a part of Image package, so I guess it would also fit directly into octave core.